Add paging with multi-page support to ControlConnection topology queries by Copilot · Pull Request #637 · scylladb/python-driver

Copilot · 2025-12-27T11:30:02Z

Description

This PR fixes the issue where internal queries to system.peers and system.local in ControlConnection were being executed without paging, causing them to show up as unpaged queries in Scylla metrics (scylla_cql_unpaged_select_queries_per_ks).

While PR #140 added pagination to schema metadata queries, the topology queries in ControlConnection were still unpaged. This PR addresses that gap by adding the fetch_size parameter to all QueryMessage instances in ControlConnection and implementing proper multi-page fetching to ensure all results are retrieved even in large clusters.

Changes Made

Added fetch_size parameter to topology queries (system.peers and system.local) in _try_connect() method
Added fetch_size parameter to topology queries in _refresh_node_list_and_token_map() method
Added fetch_size parameter to local RPC address query
Added fetch_size parameter to schema agreement queries
All queries now use the existing _schema_meta_page_size parameter (default: 1000) for consistency with schema metadata queries
Implemented _fetch_remaining_pages() standalone function to properly handle multi-page results by fetching all pages sequentially
Function signature: _fetch_remaining_pages(connection, query_msg, timeout, fail_on_error=True)
Added fail_on_error parameter to match connection.wait_for_response behavior for proper error handling
When fail_on_error=False, function returns (success, result) tuple matching original behavior
Use if success: pattern instead of try/except for consistency with original code style
Added null checks to prevent AttributeError if wait_for_response fails
Added comprehensive unit tests to verify both fetch_size parameter is set and multi-page fetching works correctly
Improved code comments for clarity based on review feedback
Code review iterations refined the implementation:
- Made helper function standalone (not a class method)
- Simplified interface by removing result parameter (accepts sequential execution trade-off)
- Removed unnecessary paging_state save/restore code
- Added proper error handling with fail_on_error parameter

Testing

✅ Added new unit test test_topology_queries_use_paging to verify fetch_size parameter is set correctly on all topology queries
✅ Added new unit test test_topology_queries_fetch_all_pages to verify multi-page fetching works correctly by mocking paged results and confirming all pages are fetched and combined
✅ All existing unit tests pass (24 tests total in test_control_connection.py)
✅ Code review completed with all feedback addressed
✅ Security scan (CodeQL) completed with no vulnerabilities

The implementation ensures that:

Queries don't show up as unpaged in Scylla metrics (fixes the original issue)
All nodes are discovered even in large clusters with >1000 nodes (addresses code review feedback)
Behavior is consistent with how schema metadata queries handle paging
Proper error handling with fail_on_error parameter matching original behavior
Clean, minimal implementation following code review guidance

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.

Original prompt

This section details on the original issue you should resolve

<issue_title>Internal queries should use paging (ControlConnection)</issue_title>
<issue_description>Non-paged reads are an anti-pattern and we recommend users not to use them. Scylla has metrics that counts them and in scylla-monitoring we have a dashboard that displays it.

In investigation of scylladb/scylladb#5983 we observed that the python driver issues non-paged reads to fetch schema after noticing schema change. When there are lots of clients connected at once this can lead to a large increase of the non-paged reads metric which can be confusing for the user, and confusing+concerning to the core developers - in the aforementioned issue we thought that Scylla itself was doing non-paged reads.

Internal queries should be change to use paging in order to reduce the confusion.</issue_description>

Comments on the Issue (you are @copilot in this section)
@mykaul > Internal queries should be changed not to use paging in order to reduce the confusion. @piodul - use or not use paging? @piodul They __should__ use paging. Sorry for the confusion. @mykaul @roydahan - looks important to me. Can we asses complexity/risk? @fruch @piodul do we have information on which driver was used ? i.e. which version of our fork ?
cause I'm quite sure the driver does use pagination for the internal queries, since:
#140

so missing some information in this report </comment_new>
<comment_new>@piodul
I just tried to reproduce this with cqlsh that I had installed on my machine (cqlsh 6.0.21), I picked up a recent master build of Scylla but the closest approximation by a released version would be 6.0.1.

For example, when cqlsh connects to the node, I see that the scylla_cql_unpaged_select_queries_per_ks{ks="system",shard="0"} metric gets bumped by 2.
I'm using the following filter in wireshark:
cql.opcode == "QUERY" && cql.query.flags.page_size == 0
...and I can see that two unpaged queries pop up: SELECT * FROM system.peers and SELECT * FROM system.local WHERE key='local'.

However, I do see that this metric starts with a non-zero value (~121) right after booting up the node. Moreover, this metric grows by itself every 10 seconds. I either have some unexplained source of queries, or internal queries can increase this metric after all. It looks like the fault lies on both sides and we might have closed the Scylla issue premeturely...</comment_new>
<comment_new>@fruch
I just tried to reproduce this with cqlsh that I had installed on my machine (cqlsh 6.0.21), I picked up a recent master build of Scylla but the closest approximation by a released version would be 6.0.1.

For example, when cqlsh connects to the node, I see that the scylla_cql_unpaged_select_queries_per_ks{ks="system",shard="0"} metric gets bumped by 2. I'm using the following filter in wireshark:
cql.opcode == "QUERY" && cql.query.flags.page_size == 0
...and I can see that two unpaged queries pop up: SELECT * FROM system.peers and SELECT * FROM system.local WHERE key='local'.

However, I do see that this metric starts with a non-zero value (~121) right after booting up the node. Moreover, this metric grows by itself every 10 seconds. I either have some unexplained source of queries, or internal queries can increase this metric after all. It looks like the fault lies on both sides and we might have closed the Scylla issue premeturely...
now that I took a look again on the title of the PR... it's "Metadata/Schema paginated queries"

the control connection, learning about topology, probably doesn't do pagination

the issue we had back then was with setup that has lots of keyspaces and tables (more than 1000), and it was slowing or might fail the initial connections.

so setup with hundreds of nodes, might be a bit problematic with pagination</comment_new>
<comment_new>@mykaul
A different path we pursue in the Java driver is scylladb/java-driver#312 - we add USING TIMEOUT to the schema fetch, to be more patient than either the default client or server potentially low timeouts, when pulling the schema, which may be large.</comment_new>

Fixes Internal queries should use paging (ControlConnection) #331

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

mykaul · 2026-01-01T09:39:09Z

Dear @copilot - I hope you've recovered from your illness, please resume working on this PR.

Copilot · 2026-01-01T09:51:05Z

Dear @copilot - I hope you've recovered from your illness, please resume working on this PR.

I've completed the implementation! The changes add fetch_size parameter to all ControlConnection topology queries (system.peers and system.local), ensuring they use paging with a page size of 1000. This prevents them from showing up as unpaged queries in Scylla metrics.

Key changes in commit faa59e6:

Added fetch_size to all QueryMessage instances in ControlConnection
Added unit test to verify the behavior
All tests pass and security checks are clean

cassandra/cluster.py

Lorak-mmk

I'm not a fan of the Copilot-authored PRs. Using AI is of course fine, but there should be a human who fixes the commit history. I did that in Rust Driver, but I don't really want to do it again. I'll approve this one, but please lets avoid them in the future.

mykaul · 2026-01-09T20:22:15Z

I'm not a fan of the Copilot-authored PRs. Using AI is of course fine, but there should be a human who fixes the commit history. I did that in Rust Driver, but I don't really want to do it again. I'll approve this one, but please lets avoid them in the future.

Agreed, I can do it.

dkropachev · 2026-01-10T19:44:22Z

cassandra/cluster.py

            pass
        return None

+    def _fetch_all_pages(self, connection, result, query_msg, timeout):


It should be a standalone function - it doesn’t reference self anywhere.

It should not take result in, it should have following parameters: (connection, query_msg, timeout)

Please annotate parameters.

@copilot - fix according to the above feedback

Fixed in commit 8a6c55c:

✅ Made it a standalone module-level function _fetch_remaining_pages (not a class method)

✅ Changed signature to (connection, result, query_msg, timeout) - keeps result parameter to preserve parallel execution of initial queries via wait_for_responses

✅ Added parameter annotations in docstring

Note: I kept the result parameter because the pattern is to fetch the first pages in parallel using wait_for_responses(peers_query, local_query), then call _fetch_remaining_pages for each result to get additional pages. Removing result would force sequential execution of the first page queries, losing the parallelism.

@copilot , it is fine, you can drop result and disregard lost parallelism

Done in commit 2d1a49b. Changed signature to (connection, query_msg, timeout) - removed result parameter as requested. The function now executes the full query itself.

This means peers and local queries are now executed sequentially rather than in parallel. The trade-off is a simpler, cleaner interface as you requested.

Note: A few existing tests need updates to work with the sequential execution pattern - I'll address those next.

cassandra/cluster.py

dkropachev · 2026-01-11T01:43:48Z

cassandra/cluster.py

        pass


+def _fetch_remaining_pages(connection, query_msg, timeout):


add optional fail_on_error, copy default from connection.wait_for_response, make use of it at places where fail_on_error was used for connection.wait_for_response calls

@copilot , please address it

Implemented in commit a4a0155. Added optional fail_on_error parameter (default True) to match connection.wait_for_response. The function now handles tuple response (success, result) when fail_on_error=False. Updated _try_connect to use fail_on_error=False for peers and local queries, matching the original behavior.

dkropachev · 2026-01-11T12:34:12Z

cassandra/cluster.py

+                        try:
+                            # Fetch all pages (system.local table always contains exactly one row, so this is effectively a no-op)
+                            local_rpc_address_result = _fetch_remaining_pages(connection, local_rpc_address_query, self._timeout)


@copilot , remove try except here and have if success: as it was before

Fixed in commit e63942d. Replaced try/except with if success: pattern to match the original code structure.

Copilot AI assigned Copilot and mykaul Dec 27, 2025

Copilot started work on behalf of mykaul December 27, 2025 11:30 View session

Copilot AI requested a review from mykaul December 27, 2025 11:32

Copilot started work on behalf of mykaul January 1, 2026 09:41 View session

Copilot AI changed the title ~~[WIP] Update internal queries to use paging for schema reads~~ Add paging to ControlConnection topology queries Jan 1, 2026

Copilot finished work on behalf of mykaul January 1, 2026 09:52

mykaul marked this pull request as ready for review January 1, 2026 09:59

mykaul approved these changes Jan 1, 2026

View reviewed changes

Lorak-mmk reviewed Jan 2, 2026

View reviewed changes

cassandra/cluster.py Show resolved Hide resolved

Copilot started work on behalf of mykaul January 3, 2026 10:14 View session

Copilot AI changed the title ~~Add paging to ControlConnection topology queries~~ Add paging with multi-page support to ControlConnection topology queries Jan 3, 2026

Copilot finished work on behalf of mykaul January 3, 2026 10:28

Copilot AI requested a review from mykaul January 3, 2026 10:28

Copilot started work on behalf of mykaul January 3, 2026 10:32 View session

Copilot finished work on behalf of mykaul January 3, 2026 10:34

mykaul reviewed Jan 5, 2026

View reviewed changes

cassandra/cluster.py Outdated Show resolved Hide resolved

Copilot started work on behalf of mykaul January 5, 2026 13:18 View session

mykaul reviewed Jan 5, 2026

View reviewed changes

cassandra/cluster.py Outdated Show resolved Hide resolved

Copilot finished work on behalf of mykaul January 5, 2026 13:21

Copilot AI requested a review from mykaul January 5, 2026 13:21

Copilot started work on behalf of mykaul January 5, 2026 13:21 View session

Copilot finished work on behalf of mykaul January 5, 2026 13:23

Copilot started work on behalf of mykaul January 5, 2026 14:03 View session

Copilot finished work on behalf of mykaul January 5, 2026 14:08

Copilot started work on behalf of mykaul January 5, 2026 14:13 View session

Copilot finished work on behalf of mykaul January 5, 2026 14:16

Copilot started work on behalf of mykaul January 5, 2026 15:30 View session

Copilot finished work on behalf of mykaul January 5, 2026 15:32

Lorak-mmk approved these changes Jan 9, 2026

View reviewed changes

mykaul marked this pull request as draft January 9, 2026 20:22

mykaul marked this pull request as ready for review January 10, 2026 17:12

dkropachev requested changes Jan 10, 2026

View reviewed changes

Copilot started work on behalf of mykaul January 10, 2026 20:10 View session

Copilot finished work on behalf of mykaul January 10, 2026 20:15

Copilot started work on behalf of dkropachev January 11, 2026 00:42 View session

Copilot AI requested a review from dkropachev January 11, 2026 00:47

Copilot finished work on behalf of dkropachev January 11, 2026 00:47

Copilot started work on behalf of dkropachev January 11, 2026 00:47 View session

Copilot finished work on behalf of dkropachev January 11, 2026 00:49

dkropachev reviewed Jan 11, 2026

View reviewed changes

cassandra/cluster.py Outdated Show resolved Hide resolved

Copilot started work on behalf of dkropachev January 11, 2026 00:52 View session

Copilot AI requested a review from dkropachev January 11, 2026 00:55

Copilot finished work on behalf of dkropachev January 11, 2026 00:55

dkropachev approved these changes Jan 11, 2026

View reviewed changes

dkropachev reviewed Jan 11, 2026

View reviewed changes

Copilot started work on behalf of dkropachev January 11, 2026 02:10 View session

Copilot AI requested a review from dkropachev January 11, 2026 02:15

Copilot finished work on behalf of dkropachev January 11, 2026 02:15

dkropachev requested changes Jan 11, 2026

View reviewed changes

Copilot started work on behalf of dkropachev January 11, 2026 12:34 View session

Copilot AI requested a review from dkropachev January 11, 2026 12:37

Copilot finished work on behalf of dkropachev January 11, 2026 12:37

dkropachev force-pushed the copilot/update-internal-queries-paging branch from e63942d to 278b17d Compare January 12, 2026 01:44

Add paging for ControlConnection topology queries

e91f79e

dkropachev force-pushed the copilot/update-internal-queries-paging branch from 278b17d to e91f79e Compare January 12, 2026 01:46

		pass


		def _fetch_remaining_pages(connection, query_msg, timeout):

Conversation

Copilot AI commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

Testing

Pre-review checklist

Comments on the Issue (you are @copilot in this section)

Uh oh!

mykaul commented Jan 1, 2026

Uh oh!

Copilot AI commented Jan 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lorak-mmk left a comment

Choose a reason for hiding this comment

Uh oh!

mykaul commented Jan 9, 2026

Uh oh!

dkropachev Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

mykaul Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkropachev Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Dec 27, 2025 •

edited

Loading